Skip to content

Comments

implement minmax intrinsics#49249

Merged
bors merged 6 commits intorust-lang:masterfrom
gnzlbg:simd_minmax
Mar 27, 2018
Merged

implement minmax intrinsics#49249
bors merged 6 commits intorust-lang:masterfrom
gnzlbg:simd_minmax

Conversation

@gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Mar 21, 2018 •
edited
Loading

This adds the simd_{fmin,fmax} intrinsics, which do a vertical (lane-wise) min/max for floating point vectors that's equivalent to Rust's min/max for f32/f64.

It might make sense to make {f32,f64}::{min,max} use the minnum and minmax intrinsics as well.

HELP: I need some help with these. Either I should go to sleep or there must be something that I must be missing. AFAICT I am calling the maxnum builder correctly, yet rustc/LLVM seem to insert a call to llvm.minnum there instead... EDIT: Rust's LLVM version is too old :/

@rust-highfive
Copy link
Contributor

rust-highfive commented Mar 21, 2018

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2018
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Mar 21, 2018

I am not insane! The LLVM source code looked correct, but a blame revealed that this was actually fixed in LLVM trunk 29 days ago and that it was broken before that: llvm-mirror/llvm@e271f8c

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Mar 21, 2018

@alexcrichton we could provide a correct implementation in stdsimd without these intrinsics and update those once we update to the next LLVM version. In the meantime, that might generate pretty horrible code. An alternative might be to backport that patch to our LLVM version.

@alexcrichton
Copy link
Member

alexcrichton commented Mar 21, 2018

Ah yeah sounds like we should provide unstable versions in stdsimd with suboptimal codegen for now?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Mar 21, 2018

I'll update this PR to make maxnum a run-fail test so that when LLVM is updated it automatically pases.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Mar 22, 2018

So:

  • I've filled a rust-lang/rust issue to track this: LLVM's maxnum builder is broken in LLVM6 #49261
  • Added the run-fail tests: these will start passing automatically once we move to LLVM 7
  • Added a codegen test for minnum, commented out the codegen for maxnum - that will need to be updated once we move to LLVM 7

@alexcrichton
Copy link
Member

alexcrichton commented Mar 22, 2018

@bors: r+

@bors
Copy link
Collaborator

bors commented Mar 22, 2018

📌 Commit 88268d7 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 22, 2018
@kennytm
Copy link
Member

kennytm commented Mar 22, 2018

@bors r-

Unreachable expression in rustc_trans.

[00:20:22] error: unreachable expression
[00:20:22]    --> librustc_trans/builder.rs:918:13
[00:20:22]     |
[00:20:22] 918 |             instr
[00:20:22]     |             ^^^^^
[00:20:22]     |
[00:20:22] note: lint level defined here
[00:20:22]    --> librustc_trans/lib.rs:20:9
[00:20:22]     |
[00:20:22] 20  | #![deny(warnings)]
[00:20:22]     |         ^^^^^^^^
[00:20:22]     = note: #[deny(unreachable_code)] implied by #[deny(warnings)]
[00:20:22] 
[00:20:22] error: unreachable expression
[00:20:22]    --> librustc_trans/builder.rs:926:13
[00:20:22]     |
[00:20:22] 926 |             instr
[00:20:22]     |             ^^^^^
[00:20:22] 
[00:20:24] error: aborting due to 2 previous errors
[00:20:24] 
[00:20:24] error: Could not compile `rustc_trans`.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 22, 2018
@kennytm
Copy link
Member

kennytm commented Mar 22, 2018

@bors r=alexcrichton

@bors
Copy link
Collaborator

bors commented Mar 22, 2018

📌 Commit 0118a65 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 22, 2018
@kennytm
Copy link
Member

kennytm commented Mar 22, 2018

@bors r-

CI is still failing 😞. The new test requires // min-llvm-version 6.0, and maybe // ignore-emscripten too.

Details 
[00:52:50] ---- [run-fail] run-fail/simd-intrinsic-float-minmax.rs stdout ----
[00:52:50] 	
[00:52:50] error: compilation failed!
[00:52:50] status: exit code: 101
[00:52:50] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-fail/simd-intrinsic-float-minmax.rs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-fail" "--target=x86_64-unknown-linux-gnu" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-fail/simd-intrinsic-float-minmax.stage2-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Zmiri" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-fail/simd-intrinsic-float-minmax.stage2-x86_64-unknown-linux-gnu.aux"
[00:52:50] stdout:
[00:52:50] ------------------------------------------
[00:52:50] 
[00:52:50] ------------------------------------------
[00:52:50] stderr:
[00:52:50] ------------------------------------------
[00:52:50] error: internal compiler error: librustc_trans/builder.rs:918: LLVMRustBuildMinNum is not available in LLVM version < 6.0
[00:52:50] 
[00:52:50] thread 'rustc' panicked at 'Box<Any>', librustc_errors/lib.rs:543:9
[00:52:50] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:52:50] 
[00:52:50] note: the compiler unexpectedly panicked. this is a bug.
[00:52:50] 
[00:52:50] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:52:50] 
[00:52:50] note: rustc 1.26.0-dev running on x86_64-unknown-linux-gnu
[00:52:50] 
[00:52:50] note: compiler flags: -Z miri -Z unstable-options -C prefer-dynamic -C rpath
[00:52:50] 
[00:52:50] 
[00:52:50] ------------------------------------------
[00:52:50] 
[00:52:50] thread '[run-fail] run-fail/simd-intrinsic-float-minmax.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:2903:9
[00:52:50] 
[00:52:50] 
[00:52:50] failures:
[00:52:50]     [run-fail] run-fail/simd-intrinsic-float-minmax.rs

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 22, 2018
@alexcrichton
Copy link
Member

alexcrichton commented Mar 22, 2018

@bors: r+

@bors
Copy link
Collaborator

bors commented Mar 22, 2018

📌 Commit d87b403 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 22, 2018
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 23, 2018
@alexcrichton
Rollup merge of rust-lang#49249 - gnzlbg:simd_minmax, r=alexcrichton
913adba 
implement minmax intrinsics

This adds the `simd_{fmin,fmax}` intrinsics, which do a vertical (lane-wise) `min`/`max` for floating point vectors that's equivalent to Rust's `min`/`max` for `f32`/`f64`.

It might make sense to make `{f32,f64}::{min,max}` use the `minnum` and `minmax` intrinsics as well.

---

~~HELP: I need some help with these. Either I should go to sleep or there must be something that I must be missing. AFAICT I am calling the `maxnum` builder correctly, yet rustc/LLVM seem to insert a call to `llvm.minnum` there instead...~~ EDIT: Rust's LLVM version is too old :/
@alexcrichton alexcrichton mentioned this pull request Mar 23, 2018
Closed
@alexcrichton
Copy link
Member

alexcrichton commented Mar 24, 2018

@bors: r-

This hit a few failures locally in the last rollup (which didn't land):

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 24, 2018
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Mar 24, 2018 via email

@bors
Copy link
Collaborator

bors commented Mar 24, 2018

@gnzlbg: 🔑 Insufficient privileges: Not in reviewers

@bors
Copy link
Collaborator

bors commented Mar 26, 2018

☔ The latest upstream changes (presumably #49297) made this pull request unmergeable. Please resolve the merge conflicts.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Mar 26, 2018

updated and rebased

@alexcrichton
Copy link
Member

alexcrichton commented Mar 26, 2018

@bors: r+

@bors
Copy link
Collaborator

bors commented Mar 26, 2018

📌 Commit 56aaf34 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 26, 2018
@bors
Copy link
Collaborator

bors commented Mar 27, 2018

⌛ Testing commit 56aaf34 with merge 31ae4f9...

bors added a commit that referenced this pull request Mar 27, 2018
@bors
Auto merge of #49249 - gnzlbg:simd_minmax, r=alexcrichton
31ae4f9 
implement minmax intrinsics

This adds the `simd_{fmin,fmax}` intrinsics, which do a vertical (lane-wise) `min`/`max` for floating point vectors that's equivalent to Rust's `min`/`max` for `f32`/`f64`.

It might make sense to make `{f32,f64}::{min,max}` use the `minnum` and `minmax` intrinsics as well.

---

~~HELP: I need some help with these. Either I should go to sleep or there must be something that I must be missing. AFAICT I am calling the `maxnum` builder correctly, yet rustc/LLVM seem to insert a call to `llvm.minnum` there instead...~~ EDIT: Rust's LLVM version is too old :/
@bors
Copy link
Collaborator

bors commented Mar 27, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 31ae4f9 to master...

@bors bors merged commit 56aaf34 into rust-lang:master Mar 27, 2018
@gnzlbg gnzlbg mentioned this pull request Apr 6, 2018
Closed
@gnzlbg gnzlbg mentioned this pull request Jul 19, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@alexcrichton alexcrichton

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@gnzlbg @rust-highfive @alexcrichton @bors @kennytm @nikomatsakis